-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix broken parent and child aggregator #63811
Conversation
In elastic#57892 I broke *some* sub-aggregations inside of the `parent` and `child` aggregator, specifically any sub-aggregations that do work in the `postCollect` phase. This fixes it by delaying the post collect phase of aggs under `parent` and `child` until `beforeBuildingBuckets` because, well, we haven't done *any* collection until after that phase.
Pinging @elastic/es-analytics-geo (:Analytics/Aggregations) |
mappings: | ||
properties: | ||
join_field: { "type": "join", "relations": { "parent": "child", "child": "grand_child" } } | ||
id: { "type": "keyword" } | ||
|
||
- do: | ||
index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was borrowing this test setup and wanted to clean it up a bit while I was here. bulk
is ever so slightly quicker for this sort of thing. And it takes up a lot fewer lines!
--- | ||
"unconfigured": | ||
- do: | ||
index: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved this into 40_aggregations.yml
.
*/ | ||
@Override | ||
public final void postCollection() throws IOException { | ||
public void postCollection() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd be better off removing postCollection
entirely and replacing it with beforeBuildingBuckets
because it is very similar but it has access to the buckets we actually want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks @not-napoleon ! |
In elastic#57892 I broke *some* sub-aggregations inside of the `parent` and `child` aggregator, specifically any sub-aggregations that do work in the `postCollect` phase. This fixes it by delaying the post collect phase of aggs under `parent` and `child` until `beforeBuildingBuckets` because, well, we haven't done *any* collection until after that phase.
In elastic#57892 I broke *some* sub-aggregations inside of the `parent` and `child` aggregator, specifically any sub-aggregations that do work in the `postCollect` phase. This fixes it by delaying the post collect phase of aggs under `parent` and `child` until `beforeBuildingBuckets` because, well, we haven't done *any* collection until after that phase.
In #57892 I broke *some* sub-aggregations inside of the `parent` and `child` aggregator, specifically any sub-aggregations that do work in the `postCollect` phase. This fixes it by delaying the post collect phase of aggs under `parent` and `child` until `beforeBuildingBuckets` because, well, we haven't done *any* collection until after that phase.
In #57892 I broke *some* sub-aggregations inside of the `parent` and `child` aggregator, specifically any sub-aggregations that do work in the `postCollect` phase. This fixes it by delaying the post collect phase of aggs under `parent` and `child` until `beforeBuildingBuckets` because, well, we haven't done *any* collection until after that phase.
After elastic#63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in elastic#63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.
After #63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in #63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.
After elastic#63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in elastic#63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.
After #63811 it became clear to me that `postCollect` is kind of dangerous and not all that useful. So this removes it. The trouble with `postCollect` is that it all happened right after we finished calling `collect` on the `LeafBucketCollectors` but before we built the aggregation results. But in #63811 we found out that we can't call `postCollect` on the children of `parent` or `child` aggregators until we know which *which* aggregation results we're building. So this removes `postCollect` and moves all of the things we did at post-collect phase into `buildAggregations` or into hooks called in those methods.
In #57892 I broke some sub-aggregations inside of the
parent
andchild
aggregator, specifically any sub-aggregations that do work inthe
postCollect
phase. This fixes it by delaying the post collectphase of aggs under
parent
andchild
untilbeforeBuildingBuckets
because, well, we haven't done any collection until after that phase.